Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change(web): remove support for es5 🏗️ #11881

Merged
merged 15 commits into from
Jul 31, 2024

Conversation

ermshiperete
Copy link
Contributor

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S5 milestone Jun 26, 2024
@ermshiperete ermshiperete marked this pull request as ready for review July 2, 2024 17:20
@ermshiperete ermshiperete changed the base branch from master to refactor/web/typescript July 2, 2024 17:21
@ermshiperete ermshiperete marked this pull request as draft July 2, 2024 21:07
@ermshiperete ermshiperete force-pushed the change/web/11878_remove-es5 branch 2 times, most recently from 68a1330 to a1bdce0 Compare July 4, 2024 16:52
ermshiperete and others added 3 commits July 4, 2024 20:49
…'s version

Node's version has special behavior for events named 'error' - if no handler, it auto-throws.  EventEmitter3 does not include this.
This fixes building all dependencies, e.g. when running `./build.sh test`
which previously failed because it didn't build all necessary targets.
@ermshiperete ermshiperete force-pushed the change/web/11878_remove-es5 branch 2 times, most recently from c2f69cc to 2d9d847 Compare July 4, 2024 20:21
@ermshiperete ermshiperete marked this pull request as ready for review July 4, 2024 21:15
@@ -21,7 +21,7 @@ export default {
concurrency: 10,
nodeResolve: true,
files: [
'**/*.spec.ts'
'build/lib/**/*.spec.mjs'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly prefer a specific build folder for tests, then a specific folder for each test set.

Something like build/test/dom/**/*.spec.mjs - this pattern is being maintained for /web/src/test/auto/dom/ tests, so why not be consistent with the pattern and reuse it here?

(While this module's headless tests aren't yet in TS, they could then go under build/test/headless/**.)

A general pattern is for lib folders to hold bundled / fully-compiled outputs, not testing files... right? Or am I just mistaken about that? Either way, that's a pattern I've held to strongly with the lower-level packages; test file outputs do not belong in that folder, in my opinion. Too messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@jahorton jahorton Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this file exists to polyfill for things that would be otherwise missing with ES5, etc. There's a strong chance we should be able to eliminate this script entirely, though it'd take a bit of extra work.

(To be clear, that "extra work" could be done later; just make a tracking issue if so.)

common/web/tslib/README.md Outdated Show resolved Hide resolved
Comment on lines +326 to +330
if(this.listenerCount('error') > 0) {
this.emit('error', err);
} else {
throw err;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers, just in case:

We use eventemitter3 so that Web can safely compile against common/web/types; events is a Node-specific package that won't exist in the browser. There is one particular point of divergence between the two types:

  • In events, .emit('error') will actually throw the error if there are no registered listeners.
  • In eventemitter3, this specialized behavior is omitted. It will never throw on .emit('error') regardless of listener count.

This is the one spot in xml2js that uses .emit('error'), so we opted for a precise adjustment to accommodate for the difference between the two implementations that will replicate the behavior of the original. One of the common/web/types unit tests fails otherwise due to reliance on the pattern.

web/build.sh Outdated Show resolved Hide resolved
@darcywong00 darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024
Base automatically changed from refactor/web/typescript to master July 22, 2024 06:11
ermshiperete and others added 4 commits July 22, 2024 17:52
Co-authored-by: Joshua Horton <[email protected]>
This makes it easier to see which part of the build fails.
Put production module and test modules in different output subdirectories.
This addresses code review comments.
Addresses code review comment.
Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notes added here should not be considered "blockers" - but they may prove material worth a look. They're a follow-up regarding the noted filesize-increase concerns.

@@ -1,5 +1,4 @@
// Future TODO: import from @keymanapp/common-types... once we no longer need to support ES5.
import { Uni_IsSurrogate1, Uni_IsSurrogate2 } from '@keymanapp/web-utils';
import { util } from '@keymanapp/common-types';
Copy link
Contributor

@jahorton jahorton Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring this style of import for the utility methods prevents treeshaking from working as well as it could. Having to import the Uni_IsSurrogate* functions through a higher-level structure (the collective export * as util pattern) results in esbuild importing all functions exported from that structure.

Tracking issue: evanw/esbuild#1420

This can be found within the debug build of web's app/browser when built:

Warning - long code block! (click to expand)
  // ../../../../common/web/types/src/util/util.ts
  var util_exports = {};
  __export(util_exports, {
    BadStringAnalyzer: () => BadStringAnalyzer,
    BadStringType: () => BadStringType,
    CONTAINS_QUAD_ESCAPE: () => CONTAINS_QUAD_ESCAPE,
    MATCH_HEX_ESCAPE: () => MATCH_HEX_ESCAPE,
    MATCH_QUAD_ESCAPE: () => MATCH_QUAD_ESCAPE,
    NFDAnalyzer: () => NFDAnalyzer,
    StringAnalyzer: () => StringAnalyzer,
    UnescapeError: () => UnescapeError,
    Uni_IsSurrogate: () => Uni_IsSurrogate,
    Uni_IsSurrogate1: () => Uni_IsSurrogate1,
    Uni_IsSurrogate2: () => Uni_IsSurrogate2,
    boxXmlArray: () => boxXmlArray,
    describeCodepoint: () => describeCodepoint,
    escapeRegexChar: () => escapeRegexChar,
    escapeStringForRegex: () => escapeStringForRegex,
    hexOcts: () => hexOcts,
    hexQuad: () => hexQuad,
    isOneChar: () => isOneChar,
    isPUA: () => isPUA,
    isValidUnicode: () => isValidUnicode,
    toOneChar: () => toOneChar,
    unescapeOne: () => unescapeOne,
    unescapeOneQuadString: () => unescapeOneQuadString,
    unescapeQuadString: () => unescapeQuadString,
    unescapeString: () => unescapeString,
    unescapeStringToRegex: () => unescapeStringToRegex
  });
  function boxXmlArray(o, x) {
    if (typeof o == "object" && !Array.isArray(o[x])) {
      if (o[x] === null || o[x] === void 0) {
        o[x] = [];
      } else {
        o[x] = [o[x]];
      }
    }
  }
  __name(boxXmlArray, "boxXmlArray");
  var MATCH_HEX_ESCAPE = /\\u{([0-9a-fA-F ]{1,})}/g;
  var CONTAINS_QUAD_ESCAPE = /(?:\\u([0-9a-fA-F]{4})|\\U([0-9a-fA-F]{8}))/;
  var MATCH_QUAD_ESCAPE = new RegExp(CONTAINS_QUAD_ESCAPE, "g");
  var _UnescapeError = class _UnescapeError extends Error {
  };
  __name(_UnescapeError, "UnescapeError");
  var UnescapeError = _UnescapeError;
  function unescapeOne(hex) {
    const codepoint = Number.parseInt(hex, 16);
    return String.fromCodePoint(codepoint);
  }
  __name(unescapeOne, "unescapeOne");
  function unescapeOneQuadString(s) {
    if (!s || !s.match(MATCH_QUAD_ESCAPE)) {
      throw new UnescapeError(`Not a quad escape: ${s}`);
    }
    function processMatch(str, m16, m32) {
      return unescapeOne(m16 || m32);
    }
    __name(processMatch, "processMatch");
    s = s.replace(MATCH_QUAD_ESCAPE, processMatch);
    return s;
  }
  __name(unescapeOneQuadString, "unescapeOneQuadString");
  function unescapeQuadString(s) {
    s = s.replaceAll(MATCH_QUAD_ESCAPE, (quad) => unescapeOneQuadString(quad));
    return s;
  }
  __name(unescapeQuadString, "unescapeQuadString");
  function unescapeString(s) {
    if (!s) {
      return s;
    }
    try {
      let processMatch2 = function(str, matched) {
        const codepoints = matched.split(" ");
        const unescaped = codepoints.map(unescapeOne);
        return unescaped.join("");
      };
      var processMatch = processMatch2;
      __name(processMatch2, "processMatch");
      s = s.replaceAll(MATCH_HEX_ESCAPE, processMatch2);
    } catch (e) {
      if (e instanceof RangeError) {
        throw new UnescapeError(`Out of range while unescaping '${s}': ${e.message}`, { cause: e });
      } else {
        throw e;
      }
    }
    return s;
  }
  __name(unescapeString, "unescapeString");
  function hexQuad(n) {
    if (n < 0 || n > 65535) {
      throw RangeError(`${n} not in [0x0000,0xFFFF]`);
    }
    return n.toString(16).padStart(4, "0");
  }
  __name(hexQuad, "hexQuad");
  function hexOcts(n) {
    if (n < 0 || n > 4294967295) {
      throw RangeError(`${n} not in [0x00000000,0xFFFFFFFF]`);
    }
    return n.toString(16).padStart(8, "0");
  }
  __name(hexOcts, "hexOcts");
  function escapeRegexChar(ch) {
    const code = ch.codePointAt(0);
    if (code <= 65535) {
      return "\\u" + hexQuad(code);
    } else {
      return "\\U" + hexOcts(code);
    }
  }
  __name(escapeRegexChar, "escapeRegexChar");
  var REGEX_SYNTAX_CHAR = /^[\u0000-\u001F\u007F-\u009F{}\[\]\\?|.^$*()/+-]$/;
  function escapeRegexCharIfSyntax(ch) {
    if (REGEX_SYNTAX_CHAR.test(ch) || !isValidUnicode(ch.codePointAt(0))) {
      return escapeRegexChar(ch);
    } else {
      return ch;
    }
  }
  __name(escapeRegexCharIfSyntax, "escapeRegexCharIfSyntax");
  function regexOne(hex) {
    const unescaped = unescapeOne(hex);
    return Array.from(unescaped).map((ch) => escapeRegexCharIfSyntax(ch)).join("");
  }
  __name(regexOne, "regexOne");
  function escapeStringForRegex(s) {
    return s.split("").map((ch) => escapeRegexCharIfSyntax(ch)).join("");
  }
  __name(escapeStringForRegex, "escapeStringForRegex");
  function unescapeStringToRegex(s) {
    if (!s) {
      return s;
    }
    try {
      let processMatch2 = function(str, matched) {
        const codepoints = matched.split(" ");
        const unescaped = codepoints.map(regexOne);
        return unescaped.join("");
      };
      var processMatch = processMatch2;
      __name(processMatch2, "processMatch");
      s = s.replaceAll(MATCH_HEX_ESCAPE, processMatch2);
    } catch (e) {
      if (e instanceof RangeError) {
        throw new UnescapeError(`Out of range while unescaping '${s}': ${e.message}`, { cause: e });
      } else {
        throw e;
      }
    }
    return s;
  }
  __name(unescapeStringToRegex, "unescapeStringToRegex");
  function isOneChar(value) {
    return [...value].length === 1;
  }
  __name(isOneChar, "isOneChar");
  function toOneChar(value) {
    if (!isOneChar(value)) {
      throw Error(`Not a single char: ${value}`);
    }
    return value.codePointAt(0);
  }
  __name(toOneChar, "toOneChar");
  function describeCodepoint(ch) {
    let s;
    const p = BadStringAnalyzer.getProblem(ch);
    if (p != null) {
      s = p;
    } else {
      s = `"${String.fromCodePoint(ch)}"`;
    }
    return `${s} (U+${Number(ch).toString(16).toUpperCase()})`;
  }
  __name(describeCodepoint, "describeCodepoint");
  var BadStringType = /* @__PURE__ */ ((BadStringType2) => {
    BadStringType2["pua"] = "PUA";
    BadStringType2["unassigned"] = "Unassigned";
    BadStringType2["illegal"] = "Illegal";
    BadStringType2["denormalized"] = "Denormalized";
    return BadStringType2;
  })(BadStringType || {});
  var Uni_LEAD_SURROGATE_START = 55296;
  var Uni_LEAD_SURROGATE_END = 56319;
  var Uni_TRAIL_SURROGATE_START = 56320;
  var Uni_TRAIL_SURROGATE_END = 57343;
  var Uni_SURROGATE_START = Uni_LEAD_SURROGATE_START;
  var Uni_SURROGATE_END = Uni_TRAIL_SURROGATE_END;
  var Uni_FD_NONCHARACTER_START = 64976;
  var Uni_FD_NONCHARACTER_END = 65007;
  var Uni_FFFE_NONCHARACTER = 65534;
  var Uni_PLANE_MASK = 2031616;
  var Uni_MAX_CODEPOINT = 1114111;
  var Uni_PUA_00_START = 57344;
  var Uni_PUA_00_END = 63743;
  var Uni_PUA_15_START = 983040;
  var Uni_PUA_15_END = 1048573;
  var Uni_PUA_16_START = 1048576;
  var Uni_PUA_16_END = 1114109;
  function Uni_IsSurrogate1(ch) {
    return ch >= Uni_LEAD_SURROGATE_START && ch <= Uni_LEAD_SURROGATE_END;
  }
  __name(Uni_IsSurrogate1, "Uni_IsSurrogate1");
  function Uni_IsSurrogate2(ch) {
    return ch >= Uni_TRAIL_SURROGATE_START && ch <= Uni_TRAIL_SURROGATE_END;
  }
  __name(Uni_IsSurrogate2, "Uni_IsSurrogate2");
  function Uni_IsSurrogate(ch) {
    return Uni_IsSurrogate1(ch) || Uni_IsSurrogate2(ch);
  }
  __name(Uni_IsSurrogate, "Uni_IsSurrogate");
  function Uni_IsEndOfPlaneNonCharacter(ch) {
    return (ch & Uni_FFFE_NONCHARACTER) == Uni_FFFE_NONCHARACTER;
  }
  __name(Uni_IsEndOfPlaneNonCharacter, "Uni_IsEndOfPlaneNonCharacter");
  function Uni_IsNoncharacter(ch) {
    return ch >= Uni_FD_NONCHARACTER_START && ch <= Uni_FD_NONCHARACTER_END || Uni_IsEndOfPlaneNonCharacter(ch);
  }
  __name(Uni_IsNoncharacter, "Uni_IsNoncharacter");
  function Uni_InCodespace(ch) {
    return ch >= 0 && ch <= Uni_MAX_CODEPOINT;
  }
  __name(Uni_InCodespace, "Uni_InCodespace");
  function Uni_IsValid1(ch) {
    return Uni_InCodespace(ch) && !Uni_IsSurrogate(ch) && !Uni_IsNoncharacter(ch);
  }
  __name(Uni_IsValid1, "Uni_IsValid1");
  function isValidUnicode(start, end) {
    if (!end) {
      return Uni_IsValid1(start);
    } else if (!Uni_IsValid1(end) || !Uni_IsValid1(start) || end < start) {
      return false;
    } else if (start <= Uni_SURROGATE_END && end >= Uni_SURROGATE_START) {
      return false;
    } else if (start <= Uni_FD_NONCHARACTER_END && end >= Uni_FD_NONCHARACTER_START) {
      return false;
    } else if ((start & Uni_PLANE_MASK) != (end & Uni_PLANE_MASK)) {
      return false;
    } else {
      return true;
    }
  }
  __name(isValidUnicode, "isValidUnicode");
  function isPUA(ch) {
    return ch >= Uni_PUA_00_START && ch <= Uni_PUA_00_END || ch >= Uni_PUA_15_START && ch <= Uni_PUA_15_END || ch >= Uni_PUA_16_START && ch <= Uni_PUA_16_END;
  }
  __name(isPUA, "isPUA");
  var _BadStringMap = class _BadStringMap extends Map {
    toString() {
      if (!this.size) {
        return "{}";
      }
      return Array.from(this.entries()).map(([t, s]) => `${t}: ${Array.from(s.values()).map(describeCodepoint).join(" ")}`).join(", ");
    }
  };
  __name(_BadStringMap, "BadStringMap");
  var BadStringMap = _BadStringMap;
  var _StringAnalyzer = class _StringAnalyzer {
    constructor() {
      /** internal map */
      __publicField(this, "m", new BadStringMap());
    }
    /** add a string for analysis */
    add(s) {
      for (const c of [...s]) {
        const ch = c.codePointAt(0);
        const problem = this.analyzeCodePoint(c, ch);
        if (problem) {
          this.addProblem(ch, problem);
        }
      }
    }
    /** internal interface for the result of an analysis */
    addProblem(ch, type) {
      if (!this.m.has(type)) {
        this.m.set(type, /* @__PURE__ */ new Set());
      }
      this.m.get(type).add(ch);
    }
    /** get the results of the analysis */
    analyze() {
      if (this.m.size == 0) {
        return null;
      } else {
        return this.m;
      }
    }
  };
  __name(_StringAnalyzer, "StringAnalyzer");
  var StringAnalyzer = _StringAnalyzer;
  var _BadStringAnalyzer = class _BadStringAnalyzer extends StringAnalyzer {
    /** analyze one codepoint */
    analyzeCodePoint(c, ch) {
      return _BadStringAnalyzer.getProblem(ch);
    }
    /** export analyzer function  */
    static getProblem(ch) {
      if (!isValidUnicode(ch)) {
        return "Illegal" /* illegal */;
      } else if (isPUA(ch)) {
        return "PUA" /* pua */;
      } else {
        return null;
      }
    }
  };
  __name(_BadStringAnalyzer, "BadStringAnalyzer");
  var BadStringAnalyzer = _BadStringAnalyzer;
  var _NFDAnalyzer = class _NFDAnalyzer extends StringAnalyzer {
    analyzeCodePoint(c, ch) {
      const nfd = c.normalize("NFD");
      if (c !== nfd) {
        return "Denormalized" /* denormalized */;
      } else {
        return null;
      }
    }
  };
  __name(_NFDAnalyzer, "NFDAnalyzer");
  var NFDAnalyzer = _NFDAnalyzer;

Copy link
Contributor

@jahorton jahorton Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I instead import those functions directly from the original, defining source file...

  // ../../../../common/web/types/src/util/util.ts
  var CONTAINS_QUAD_ESCAPE = /(?:\\u([0-9a-fA-F]{4})|\\U([0-9a-fA-F]{8}))/;
  var MATCH_QUAD_ESCAPE = new RegExp(CONTAINS_QUAD_ESCAPE, "g");
  var Uni_LEAD_SURROGATE_START = 55296;
  var Uni_LEAD_SURROGATE_END = 56319;
  var Uni_TRAIL_SURROGATE_START = 56320;
  var Uni_TRAIL_SURROGATE_END = 57343;
  function Uni_IsSurrogate1(ch) {
    return ch >= Uni_LEAD_SURROGATE_START && ch <= Uni_LEAD_SURROGATE_END;
  }
  __name(Uni_IsSurrogate1, "Uni_IsSurrogate1");
  function Uni_IsSurrogate2(ch) {
    return ch >= Uni_TRAIL_SURROGATE_START && ch <= Uni_TRAIL_SURROGATE_END;
  }
  __name(Uni_IsSurrogate2, "Uni_IsSurrogate2");

Way less imported code this way - easily enough to account for a few KB in file-size difference even when minified.

Alternatively, exporting the two explicitly from common/web/types/src/main.ts outside of a "namespace" export would also do the job. Could be "in addition to" to keep the overall change set minimal, rather than removing it from the util namespace currently used by other consumers.

Copy link
Contributor

@jahorton jahorton Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively... we could just leave the versions defined within common/web/utils in place, as they're not subject to the import pattern causing the issue.

Alternatively-alternatively, we could expose the util.js file as its own... "sub-export"(?) from common/web/types. It's a pattern I'm comfortable implementing if we want to go that route. We'd then be able to import { whatever } from '@keymanapp/common-types/utils' or similar. (We can choose what we name that final path component.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the alternative alternative. With that the size stays the same.

jahorton and others added 3 commits July 23, 2024 12:24
The exports of the `util.ts` can now be imported as
`@keymanapp/common-types/utils`. Those methods are used by developer.
We still export the `Uni_IsSurrogate*` methods in `main.js` because they
are needed by the keyboard-processor, but exporting everything would
increase the size of the resulting `keymanweb.js`. Doing it this way
keeps the size the same.
Missed one occurrence of `util`...
Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though we should probably get @mcdurdin's thoughts on the parts of the changes that affect common/web/types + developer.

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things I am not comfortable with here. The sharing of code common/web/types between Developer and Web seems still to be a little difficult. Perhaps we should have a discussion about that?

web/build.sh Show resolved Hide resolved
@@ -1,4 +1,5 @@
import { util, CompilerErrorNamespace, CompilerErrorSeverity, CompilerMessageSpec as m, CompilerMessageDef as def } from "@keymanapp/common-types";
import { CompilerErrorNamespace, CompilerErrorSeverity, CompilerMessageSpec as m, CompilerMessageDef as def } from "@keymanapp/common-types";
import * as util from '@keymanapp/common-types/utils';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be using deep imports between the kmc modules -- we are working towards well-defined API surfaces for each module, and this is a definite step backwards. Can we please only use top-level imports.

(Doesn't look like eslint has a good rule for this at present?)

Copy link
Contributor

@jahorton jahorton Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context: #11881 (comment)

Without this pattern, KMW will grow approximately 4 KB in filesize. That said, yeah, this file isn't part of KMW, so there's no reason to do the deep import here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw the context. I am asking that we don't fix the growth issue by introducing a new problem.

We do not need to use this pattern. We can just export the two functions that KMW needs from common-types/main.ts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to use this pattern. We can just export the two functions that KMW needs from common-types/main.ts.

I do agree with that; it does feel like the better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@jahorton jahorton Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine if the constants are the only "extras" along for the ride; their file-size contribution is minimal. This is what I see on my local build - the constants are pulled in, but they're just four extra lines, and the longest part of each is a variable name that can be substantially minified.

Here's the minified form of what we currently get:

var dQ=55296,gQ=56319,uQ=56320,IQ=57343;function vs(Q){return Q>=dQ&&Q<=gQ}l(vs,"Uni_IsSurrogate1");function Ws(Q){return Q>=uQ&&Q<=IQ}l(Ws,"Uni_IsSurrogate2")

Starting with function vs(Q), this is content we need to have anyway - that's the Uni_IsSurrogate1 definition. Before that is a total of... 40 characters. It'd be lovely if we didn't have them, but it's not worth hyper-optimizing.

Oh wait. We actually do use them, too; they're just not inlined. (Inlining would be optimal because each is only used once, but oh well.)

var dQ=55296,gQ=56319, // ...

function vs(Q){return Q>=dQ&&Q<=gQ} // ...

We wouldn't even save 40 chars in total!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to give a concise answer, I pick option 3: neither. Leave it as you currently have it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To achieve this I moved the constants to util/consts.ts.

Upon re-reading this, I now think your new point was about the regex constants. They're the only content of the file you're referencing in that quote, after all. That said... I don't see the behavior you're indicating.

Use the following two lines at the top of common/web/types/src/util/util.ts:

import { MATCH_HEX_ESCAPE, CONTAINS_QUAD_ESCAPE, MATCH_QUAD_ESCAPE } from './consts.js';
export { MATCH_HEX_ESCAPE, CONTAINS_QUAD_ESCAPE, MATCH_QUAD_ESCAPE };

The regex constants will still be available for general import where needed in the other packages this way, at their original location. Furthermore, running ./web/build.sh build:app/browser shows no sign of those regex entries within the Web build products when I run it locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing the size issue at present -- I think @jahorton's final comment above is probably the best answer (and should resolve the current build failures I hope!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that works, thanks! Strange that it makes a difference whether or not the consts are defined and exported in the same file...

developer/src/kmc-ldml/src/compiler/tran.ts Outdated Show resolved Hide resolved
developer/src/kmc-ldml/test/helpers/index.ts Outdated Show resolved Hide resolved
common/web/types/package.json Outdated Show resolved Hide resolved
developer/src/kmc-ldml/src/compiler/empty-compiler.ts Outdated Show resolved Hide resolved
@ermshiperete ermshiperete changed the title change(web): remove support for es5 change(web): remove support for es5 🏗️ Jul 30, 2024
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ermshiperete ermshiperete merged commit 10ed10d into master Jul 31, 2024
19 checks passed
@ermshiperete ermshiperete deleted the change/web/11878_remove-es5 branch July 31, 2024 15:48
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.80-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change(web): remove ES5 support
5 participants